Skip to content

JS: Add AdditionalTypeTrackingStep#1788

Merged
semmle-qlci merged 1 commit into
github:masterfrom
asger-semmle:additional-type-tracking-step
Aug 24, 2019
Merged

JS: Add AdditionalTypeTrackingStep#1788
semmle-qlci merged 1 commit into
github:masterfrom
asger-semmle:additional-type-tracking-step

Conversation

@asger-semmle

Copy link
Copy Markdown
Contributor

No description provided.

@asger-semmle asger-semmle requested a review from a team as a code owner August 21, 2019 12:45

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think it needs to be generalized more.

summary = LoadStep(prop)
)
or
any(AdditionalTypeTrackingStep st).step(pred, succ) and

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is a library function that writes a property that can be read later.
Shouldn't the step type be configurable to support that case?

Suggested change
any(AdditionalTypeTrackingStep st).step(pred, succ) and
any(AdditionalTypeTrackingStep st).step(pred, succ, summary) and

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all of our additional step mechanisms have this limitation, and we should totally fix that. I've often wanted to do something about this, but so far I haven't been able to find a robust way to do it right.

Concretely, the API should not restrict future implementation changes too much, and should be applied consistently across all the additional step mechanisms. Adding a step summary as an extra parameter might work here, but if we try the same in data-flow configurations, we will quickly run out of possible ways to overload this predicate, as we already have overloads using flow-labels as extra parameters.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Lets start with this version then, perhaps with a docstring warning about the default choice of flowstep (which all the other additional step mechanism also need).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TaintFunction API in the C++ library is a fairly nice way of supporting a special, but common, case of this. We're also using it for Go, and I think we should explore having something like it for JavaScript. (Though we'd probably have to attach the models to function calls, not the functions themselves, since we don't have those for most framework functions.)

@asger-semmle asger-semmle added this to the 1.22 milestone Aug 24, 2019
@felicitymay

felicitymay commented Aug 24, 2019

Copy link
Copy Markdown
Contributor

Change note?

@asger-semmle

Copy link
Copy Markdown
Contributor Author

Change note?

Shouldn't be necessary for this minor change.

@semmle-qlci semmle-qlci merged commit fc59dd6 into github:master Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants